Initial implementation of up-transport-vsomeip-rust #2

PLeVasseur · 2024-06-11T21:40:47Z

Hi folks!

Here's the initial implementation of a Rust up-transport wrapping vsomeip.

I'm trying to get something out the door to be able to get looked at, but note that it's still in draft status. There's still a fair number of TODOs scattered about. 😅

TODO:

Ensure we comply with the 1.5.8 spec for validation
Iron out remaining TODOs / questions around uProtocol <-> vsomeip
Add more unit tests
Add more integration tests
Modify CI to be triggered on PR to include the compiled .so for vsomeip
Update docs to note that clang required for vsomeip-sys wrapper around C++ vsomeip library

TODO: * Ensure we comply with the 1.5.8 spec for validation * Iron out remaining TODOs / questions around uProtocol <-> vsomeip * Add more unit tests * Add more integration tests * Add CI to be triggered on PR

…of headers

* Added documentation on how to build, run * Modified build process so that we can specify where the vsomeip shared libraries are located

…r purposes of getting all point-to-point messages

…g get_data_safe(), as data_ptr null means vsomeip is saying there's no data in the payload Added logging around setting & getting payload of vsomeip message

Mallets · 2024-06-13T12:43:44Z

src/determinations.rs

+    (most_significant_bits, least_significant_bits)
+}
+
+pub(crate) fn split_u32_to_u8(value: u32) -> (u8, u8, u8, u8) {


You could use https://doc.rust-lang.org/std/primitive.u32.html#method.to_le_bytes instead of manual shifting and masking.

devkelley

Some initial comments/clarifying questions

devkelley · 2024-06-13T19:49:54Z

src/lib.rs

+    #[allow(dead_code)]
+    config_path: Option<PathBuf>,
+    // if this is not None, indicates that we are in a dedicated point-to-point mode
+    point_to_point_listener: RwLock<Option<Arc<dyn UListener>>>,


Can you explain why a RwLock is needed and why you chose to wrap the Option rather than Option<RwLock<...

Sure -- I used a RwLock here because I needed a means to ensure thread-safety of the Option<> since it's used within different async functions.

devkelley · 2024-06-13T20:38:04Z

src/transport.rs

+        Ok(())
+    }
+
+    async fn register_point_to_point_listener(


One general question I have here is whether there is only one point to point listener allowed for one utransport instance?

Yes -- this is to support the uStreamer use case. If we're in that point-to-point listener mode then we will make use of the applications configured in the vsomeip config JSON file.

devkelley · 2024-06-13T20:44:08Z

src/lib.rs

+        UUri,
+        Option<UUri>,
+        RegistrationType,
+        oneshot::Sender<Result<(), UStatus>>,


can you explain what the oneshot does here? Based on my understanding of the code, its a sender that is used by the event loop to return the execution status of the TransportCommand, and since a TransportCommand only gets executed once a oneshot makes sense?

Yup, you are correct.

devkelley · 2024-06-13T20:55:11Z

src/transport.rs

+    pub(crate) static ref LISTENER_ID_MAP: ListenerIdMap = RwLock::new(HashMap::new());
+}
+
+generate_message_handler_extern_c_fns!(10000);


My understanding is that this macro creates the 'FREE_LISTENER_IDS' lazy static map, which is used to cap the number of listeners for the client and ensure that each listener has a unique id?

You can check the proc macro crate's definition of generate_message_handler_extern_c_fns. Long story short is that since an extern "C" fn is required by the vsomeip library to register a message handler function, we must pre-create all these extern "C" fns.

Internal to each extern "C" fn they call call_shared_extern_fn function which will then use the map to pick out the correct UListener to trigger.

devkelley · 2024-06-13T20:59:00Z

src/transport.rs

+        {
+            let mut listener_client_id_mapping = LISTENER_CLIENT_ID_MAPPING.write().await;
+            listener_client_id_mapping.remove(&listener_id);
+        }


should these writes all happen in the same block since they are all related to the same unregister request? I would be a bit worried that there is a scenario where a mismatch could happen.

Hmmm, okay, I'll think on it.

Decided to group these together based on your feedback.

PLeVasseur · 2024-06-14T18:38:05Z

src/lib.rs

+const UP_CLIENT_VSOMEIP_FN_TAG_INITIALIZE_NEW_APP_INTERNAL: &str = "initialize_new_app_internal";
+const UP_CLIENT_VSOMEIP_FN_TAG_START_APP: &str = "start_app";
+
+// TODO: Revisit what authority to attach, if any, to the remote mE. For now, just assign "me_authority"


@stevenhartley -- SOME/IP doesn't have a concept of an authority, so here I've chosen to attach this.

Any thoughts?

Authority in the case of SOME/IP devices will refer the IP addresses of the devices that the mechatronics services run on. We are either going to use the static IP address or the hostnames (TBD) but we need to fetch that information (it is sen din the offer_service() AFAIK).

PLeVasseur · 2024-06-14T20:28:44Z

src/lib.rs

+                    UP_CLIENT_VSOMEIP_FN_TAG_REGISTER_LISTENER_INTERNAL,
+                );
+                let (_, service_id) = split_u32_to_u16(source_filter.ue_id);
+                // let instance_id = vsomeip::ANY_INSTANCE; // TODO: Set this to 1? To ANY_INSTANCE?
